Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TS Clean-up - Move Classes and Rename Packages #660

Merged
merged 3 commits into from
May 27, 2022

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented May 25, 2022

Summary

As a part of the Test Suite clean up, classes has been moved to a packages named according to io.quarkus.ts.MODULE_NAME.SCENARIO_NAME. Exact rules how to translate MODULE_NAME to java package name has been relaxed in order to make the package names readable. That's compliant with the existing state that either merge module name to one worded package name, or join module names by dots.

Also fixed tests ChuckNorrisResourceIT#endpointShouldTrace and ChuckNorrisResourceIT#httpClientShouldHaveHisOwnSpan as Jagger service vertx-web-client is tracing operating called /trace/ping, however tests were looking for trace/ping operation and thus failed. Similarly, io.quarkus.ts.http.graphql.telemetry.TelemetryIT#verifyTelemetry operation name has been changed to /graphql, which make the test pass.

Keycloak version used in DevModeKeycloakDevServiceUserExperienceIT is upgraded to 18.0. as upstream docs says In Keycloak 17 the default distribution is now powered by Quarkus, while the legacy WildFly powered distribution will still be around until June 2022 we highly recommend starting the migration as soon as possible. and using Keycloak 13.0.1 caused test failures of DevModeKeycloakDevServiceUserExperienceIT.

Please select the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • Dependency update
  • New scenario (non-breaking change which adds functionality)
  • This change requires a documentation update
  • Refactoring

Checklist:

  • Methods and classes used in PR scenarios are meaningful
  • Commits are well encapsulated and follow the best practices

@michalvavrik michalvavrik requested a review from pjgg May 25, 2022 21:24
@michalvavrik michalvavrik self-assigned this May 25, 2022
@michalvavrik
Copy link
Member Author

cc @jsmrcka

@michalvavrik michalvavrik force-pushed the feature/package-names-part-one branch 2 times, most recently from f264296 to d560fc7 Compare May 26, 2022 07:12
Copy link
Contributor

@pjgg pjgg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also could you double-check the following modules:

  • http/jaxrs: io.quarkus.ts.security.core ?
  • http/http-static: io.quarkus.ts.http -> io.quarkus.ts.http.static
  • http/reactive-routes: io.quarkus.ts -> io.quarkus.ts.http.reactiveroutes
  • http/server-undertow: io.quarkus.ts.filters -> io.quarkus.ts.http.undertow.filters (aplies to all packages)
  • http/vertx-web-client: io.quarkus.qe.vertx -> io.quarkus.ts.http.vertx (applies to all packages)
  • infinispan-client: io.quarkus.ts.infinispan -> io.quarkus.ts.http.infinispan
    ...and so on (in general try to add the module name to the packages definition)
    io.quarkus.ts.MODULE_NAME.SCENARIO_NAME

@michalvavrik michalvavrik force-pushed the feature/package-names-part-one branch from d560fc7 to d5191f6 Compare May 26, 2022 10:25
@michalvavrik
Copy link
Member Author

Also could you double-check the following modules:

  • http/jaxrs: io.quarkus.ts.security.core ?
  • http/http-static: io.quarkus.ts.http -> io.quarkus.ts.http.static
  • http/reactive-routes: io.quarkus.ts -> io.quarkus.ts.http.reactiveroutes
  • http/server-undertow: io.quarkus.ts.filters -> io.quarkus.ts.http.undertow.filters (aplies to all packages)
  • http/vertx-web-client: io.quarkus.qe.vertx -> io.quarkus.ts.http.vertx (applies to all packages)
  • infinispan-client: io.quarkus.ts.infinispan -> io.quarkus.ts.http.infinispan
    ...and so on (in general try to add the module name to the packages definition)
    io.quarkus.ts.MODULE_NAME.SCENARIO_NAME

I didn't get that far as I tried to keep a commit at reasonable length. If though changes are insignificant, it already changes 105 files in some way. Sure, I'll address this in this commit and get back to you, thanks for suggestion

@michalvavrik michalvavrik force-pushed the feature/package-names-part-one branch 2 times, most recently from d5705c8 to 34906ae Compare May 26, 2022 16:41
@michalvavrik
Copy link
Member Author

  • http/http-static: io.quarkus.ts.http -> io.quarkus.ts.http.static
    That's not a valid java package name.

@michalvavrik michalvavrik force-pushed the feature/package-names-part-one branch from 34906ae to c2a6aca Compare May 26, 2022 16:51
@michalvavrik
Copy link
Member Author

@pjgg I have run a tests before and after every refactoring in order to determine whether I brought in any new failures. The failures can be linked to the failures in Daily runs and IMHO should be resolved separately (a big commit as this is asking for a merge hell).

@michalvavrik michalvavrik requested a review from pjgg May 26, 2022 16:53
@michalvavrik
Copy link
Member Author

Please note I also updated PR description. There is other work on clean up that should be done in a separate PRs.

@michalvavrik michalvavrik changed the title TS Clean-up - Move Sql & Websockets packages and rename tests TS Clean-up - Move Classes and Rename Packages May 26, 2022
Copy link
Contributor

@pjgg pjgg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cosmetic changes

@michalvavrik michalvavrik force-pushed the feature/package-names-part-one branch from c2a6aca to 0a41e25 Compare May 27, 2022 08:48
@michalvavrik
Copy link
Member Author

michalvavrik commented May 27, 2022

@pjgg you're absolutely right, I reverted unnecessary changes. The feedback has been very much appreciated.

@michalvavrik michalvavrik requested a review from pjgg May 27, 2022 08:52
@michalvavrik michalvavrik force-pushed the feature/package-names-part-one branch from 0a41e25 to 73e912e Compare May 27, 2022 09:30
Copy link
Contributor

@pjgg pjgg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michalvavrik
Copy link
Member Author

Fixed typo, thnx

@michalvavrik michalvavrik force-pushed the feature/package-names-part-one branch from 58d6620 to 57b2c12 Compare May 27, 2022 12:28
We should use latest Keycloak version. Upstream docs says `In Keycloak 17 the default distribution is now powered by Quarkus, 
while the legacy WildFly powered distribution will still be around until
 June 2022 we highly recommend starting the migration as soon as 
possible.` Using Keycloak 13.0.1 caused test failures of DevModeKeycloakDevServiceUserExperienceIT
Tracing operation name is registered as `/graphql` and but the test used operation name `graphql`, thus no tracing were found and the test failed.
@pjgg
Copy link
Contributor

pjgg commented May 27, 2022

Kafka errors are not related to this PR.

@pjgg pjgg merged commit 7b29d43 into quarkus-qe:main May 27, 2022
@michalvavrik michalvavrik deleted the feature/package-names-part-one branch May 27, 2022 17:05
@rsvoboda
Copy link
Member

@michalvavrik just fyi:ChuckNorrisResourceIT changes for span name were trigger by quarkusio/quarkus#24017

@rsvoboda rsvoboda added the triage/backport-2.7? It needs to backport changes to branch 2.7 label May 31, 2022
@@ -101,7 +101,7 @@ public void getTimeoutWhenResponseItsTooSlow() {
@Test
public void endpointShouldTrace() {
final int pageLimit = 50;
final String expectedOperationName = "trace/ping";
final String expectedOperationName = "/trace/ping";
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjgg this line can't be backported for only 2.8.0.CR1 + is affected.

@pjgg pjgg added this to the 2.7 milestone Jun 6, 2022
@pjgg pjgg added triage/backport-2.7? It needs to backport changes to branch 2.7 and removed triage/backport-2.7? It needs to backport changes to branch 2.7 labels Jun 6, 2022
@pjgg pjgg mentioned this pull request Jun 6, 2022
2 tasks
@pjgg pjgg removed the triage/backport-2.7? It needs to backport changes to branch 2.7 label Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants